Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade OpenGL backend to ES 3.0 #995

Merged
merged 12 commits into from
Apr 13, 2023
Merged

Upgrade OpenGL backend to ES 3.0 #995

merged 12 commits into from
Apr 13, 2023

Conversation

mwilsnd
Copy link
Collaborator

@mwilsnd mwilsnd commented Apr 10, 2023

This PR upgrades the shaders and GL contexts of all platforms (minus darwin) to OpenGL ES 3.0. A large chunk of changes here are in the shaders & generated shader code (mbgl/shaders/, shaders/) where the following changes were made:

  • #version 300 es support
    • attribute -> layout (location = n) in
    • varying -> in/out
    • texture2D -> texture
    • gl_FragColor -> explicit output color
    • The shader code generator was upgraded to emit vertex attribute layout qualifiers

All instances of MBGL_USE_GLES2 have been removed, with older pathways for ES 1 stuff deleted. All context creation methods have been updated to request ES 3 or ES 3 compatible (windows) contexts, save for macOS's CoreGL backend (support here is pending, may be done via ANGLE in the future).

@mwilsnd mwilsnd self-assigned this Apr 10, 2023
@mwilsnd mwilsnd changed the title Gles3 Upgrade OpenGL backend to ES 3.0 Apr 10, 2023
Copy link
Collaborator

@sjg-wdw sjg-wdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see the 3.0 syntax in the shaders.

Copy link
Collaborator

@TimSylvester TimSylvester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merges and works fine with my pending changes 👍

@mwilsnd mwilsnd marked this pull request as ready for review April 11, 2023 17:13
@mwilsnd mwilsnd requested review from louwers and ovivoda April 11, 2023 17:14
ntadej
ntadej previously requested changes Apr 11, 2023
Copy link
Collaborator

@ntadej ntadej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me open a pending review before I have the chance to test Qt, should be soon.

@ntadej
Copy link
Collaborator

ntadej commented Apr 11, 2023

No, Qt does not work anymore on macOS. I guess then Node.js would also be broken. What's the motivation to move to GLES 3.0?

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Apr 11, 2023

No, Qt does not work anymore on macOS. I guess then Node.js would also be broken. What's the motivation to move to GLES 3.0?

Any platform built for macOS is going to fail with this PR - Qt, node, render-test-runner and so on. Moving to ES 3 gives us a few important features, the two big ones are uniform buffer objects (UBOs) and Vertex array objects (VAOs). Using these, we can significantly simplify the existing GL rendering code and bring it more inline with what we anticipate the coming Metal backend will look like. ES 3 also brings increased minimums on certain implementation limits like number of vertex attributes, which increases to 16 with ES 3, removing one of the reasons for keeping the existing shader permutation system around.

The current plan then, is merge this PR and shut off macOS on CI. Existing users on macOS will have to remain on the last stable release until the Metal platform is ready later this year. Once Metal is ready, the darwin platform (and all dependents like glfw, Qt, node) can be moved to the new native backend and revived.

The other option is to bridge the gap with ANGLE on mac, but this is extra temporary work until Metal is implemented. This option should only be entertained if there is significant enough demand for it.

@ntadej
Copy link
Collaborator

ntadej commented Apr 11, 2023

OK, then we should cut releases before this is merged. Also this motivates more repo split as I effectively can't develop anymore mac being my primary platform.

To be safe, we should also manually test other platforms I guess (Windows is another platform I'm worried about).

Let's discuss this tomorrow how to best proceed (and if you're willing to eventually help supporting Metal in Qt 😉).

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Apr 11, 2023

OK, then we should cut releases before this is merged. Also this motivates more repo split as I effectively can't develop anymore mac being my primary platform.

To be safe, we should also manually test other platforms I guess (Windows is another platform I'm worried about).

Let's discuss this tomorrow how to best proceed (and if you're willing to eventually help supporting Metal in Qt 😉).

Windows works fine - Pictured is the GLFW platform running on Windows. Test runners also "work" (The mapbox filesystem impl doesn't support windows dir separators and explodes trying to find test metrics).
image

@sjg-wdw
Copy link
Collaborator

sjg-wdw commented Apr 11, 2023

Does Qt not support Metal?

@ntadej
Copy link
Collaborator

ntadej commented Apr 11, 2023

Qt6 fully supports Metal (Qt5 I'd have to check). The problem is I barely know bits of OpenGL so I'll need help gluing things together. Unfortunately they're missing some Metal examples that they have for Vulcan and D3D11. It will probably be minimal work for an expert though.

@TimSylvester
Copy link
Collaborator

It looks like there are new definitions that should be added to mbgl/gl/defines.hpp, e.g., GL_UNSIGNED_INT_VEC2 from https://docs.gl/es3/glGetActiveAttrib.

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Apr 12, 2023

It looks like there are new definitions that should be added to mbgl/gl/defines.hpp, e.g., GL_UNSIGNED_INT_VEC2 from https://docs.gl/es3/glGetActiveAttrib.

Being addressed in #1007

@louwers
Copy link
Collaborator

louwers commented Apr 13, 2023

@ntadej OK to merge?

@louwers louwers enabled auto-merge (squash) April 13, 2023 16:06
@louwers louwers requested a review from ntadej April 13, 2023 16:08
@ntadej
Copy link
Collaborator

ntadej commented Apr 13, 2023

First the CI should be green (so failing tests disabled) 😊

We may want to tag some releases before we merge this though. I'll be travelling tomorrow but I will be available on the weekend. I think @acalcutt also wanted to tag a release before this is merged.

Can this wait a few days? I can also merge it when green when I'm ready (and the CI is green).

@louwers
Copy link
Collaborator

louwers commented Apr 13, 2023

Can this wait a few days?

Not really I think. This is a blocker for quite a lot of other things. (cc @sjg-wdw)

If you want to make a release, it's possible to do it from the opengl-2 branch?

@acalcutt
Copy link
Collaborator

I tried to do the last node release i wanted to do but it failed to bump the version. wonder if that node-pregyp-githib secret expired @birkskyum ?

@ntadej
Copy link
Collaborator

ntadej commented Apr 13, 2023

OK, I can also tag from some older commit the way how the release is set-up (only the changelog may not be OK). So from my side you're OK to proceed.

@birkskyum
Copy link
Member

birkskyum commented Apr 13, 2023

@acalcutt it hasn't expired

@birkskyum
Copy link
Member

birkskyum commented Apr 13, 2023

@acalcutt , unless github has made some updates to their personal token settings (.. they do work on fine-grained repo-scoped personal tokens), my best guess is that it's caused by the branch protection in this repo. Currently the branch protection setting for main has enabled "Do not allow bypassing the above settings", which is good practice, but if I don't remember wrong the node deployment rely on auto-pushing a release commit directly to main effectively bypassing the PR requirement.

@acalcutt
Copy link
Collaborator

Before we were using that secret to get passed the branch protection on the version bump, since we checked out with that user also. must not be working anymore. ill have to look into it and see if i have the same issue on my fork. not sure the workflow is set up to build a release for a branch..hmm.

probably wont be able to look until later

@louwers
Copy link
Collaborator

louwers commented Apr 13, 2023

@ntadej We had a discussion about keeping macOS CI running on main.

In the end @mwilsnd, @alexcristici and me decided against it. We can apply some tricks to make the project build (but not run) for macOS on main, but this creates the false expectation that macOS will work on main. As we decided during the TSC meeting, until the Metal migration is complete macOS will be broken on main and only functioning on the opengl-2 branch. Since macOS support is one of the north stars of the design proposal and thus one of the deliverables of the team, I think we can be confident that the team will get to bringing back macOS support in due time.

Hope you can agree with this reasoning. Maybe you can approve the PR explicitly, otherwise I can't merge (without dismissing your review).

@birkskyum
Copy link
Member

@acalcutt , the token used on the fork, can you see what permissions it has? The one used here has only public_repo, repo_deployment, and I actually wouldn't expect that to work because they are read-only... I wonder if they changed this list of permissions. Would expect something like write:repo_hook to be needed

@louwers louwers dismissed ntadej’s stale review April 13, 2023 21:24

"OK to proceed."

@louwers louwers merged commit 4828cc7 into maplibre:main Apr 13, 2023
@mwilsnd mwilsnd deleted the gles3 branch April 14, 2023 17:19
TimSylvester added a commit to WetDogWeather/maplibre-gl-native that referenced this pull request Apr 14, 2023
…awable-1

* commit 'f100a4c5d6449307547c2da850db04b12c6a3a95':
  Minor reorg of defines.
  Removed more commented code.
  Removed commented code.
  Upgrade OpenGL backend to ES 3.0 (maplibre#995)
  Run macOS workflows only on `opengl-2` branch (maplibre#1012)
  Roll back linux metrics submit along with macos metrics. (maplibre#1009)
@tdcosta100
Copy link
Collaborator

tdcosta100 commented Apr 26, 2023

Hi guys. I know it's been a while since this PR was merged, but platform/windows/FindOSMesa.cmake doesn't fit these changes, because Mesa3D doesn't support GLES 3.0, only GLES 1.1 and GLES 2.0.

I'm planning to submit a PR to fix this, please validate my plans:

  1. Drop Mesa3D support in main branch, since the minimum supported version is 3.0
  2. Move/keep Mesa3D support to opengl-2 branch

Is this the correct way of doing this change?

Thank you in advance.

@acalcutt
Copy link
Collaborator

@tdcosta100 , does that depend on the driver used? from this page it seems like mesa does support ES 3.0

https://mesamatrix.net/

@tdcosta100
Copy link
Collaborator

Nice, I will try to test it creating a GLES 3 surface. If it works, I will just adjust it (Mesa3D doesn't have a libGLESv3.dll file).

@tdcosta100
Copy link
Collaborator

Yeah, it works, but interesting is: the DLL name is libGLESv2.dll, even it holds all the ES 3.0 functions. I will submit a new PR correcting this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants